this is not defined anywhere else in the Flow Aggregator code? I am just worried about keeping things consistent between the FA code and the test code.

the struct is defined in clickhouseclient.go, but:

• the Json tag binding is only used for test code. Not sure if it makes sense to include those tags there just for the sake of this e2e testing unmarshalling
• (nit) attrs are not exported / public in that file
• certain db side columns are not used as part of client (TimeInserted, Trusted), thus not included under fa.

I don't think there's too much overhead in having two structs here. If anything new is added on the FA side, the test code should be updated to take those in anyways.

The JSON tags would definitely not be an issue, however it sounds like there are other reasons for keeping them separate.

Is this a problem for regular users as well (outside of test code)? If yes, is this documented and did we consider having 2 YAML manifests instead to reduce the likelihood of the issue?

it's two steps with two yaml files in the user facing documentation.

I think that's one more argument in favor of deploying CH ou-of-band from the test cases.

My preference would have been to refactor the test code so that for these tests we deploy Antrea, the FA, and ClickHouse "out-of-band" (so not as part of the test cases themselves). I think it tends to be a better and simpler approach. However, that doesn't exactly match how we have designed tests so far, so we can think about doing it as a follow-up.

these parameter names are too vague
maybe add the URL suffix to them

done

I don't understand why we need so much customization compared to dev mode. Maybe it indicates that something should be improved?

Have you given any thought to what this will be like when the visibility solution is moved to the theia repo? Maybe you intend to improve things at that time?

essentially what we're doing here is:

• re-create a kustomize folder to use kustomization-e2e.yml as base kustomization.yml, which only includes clickhouse db but not other grafana related manifest components
• since we don't want to dup other shared files (patches / configs / db init script) in the flow-visibility dir, we're copying them from the base dir instead of building directly in its dedicated base dir. This makes it look like having a lot of customization, but actually half of the steps are just copying existing files.
• also includes clickhouse-operator-install-bundle.yml in one yaml file for easier manifest copying and deploying with test code. For release yaml it's a separate file.

Once FV manifest is moved out of Antrea we'll have a simplified folder structure dedicated for e2e test. At that time all these copying is not necessary.

I just realized that these values (for flowCollector.activeFlowExportTimeout & flowCollector.idleFlowExportTimeout) don't really make sense outside of tests, yet we are applying them every time we generate the manifest in 'dev' mode.

Maybe you could modify generate-manifest.sh so it can take as parameters extra Helm values, or the path to an Helm values file. Alternatively you could generate the manifest you need using Helm directly, with your own values file, and without using generate-manifest.sh at all.

added a new param --extra-helm-values-file for generate-manifest.sh and defined these test-facing values in a separate file.

we also have a version of this manifest checked-in in this repo, maybe it makes more sense to use our version for consistency?

separated current fv deployment step into 2. first with the install bundle already checked-in in antrea, and then the clickhouse related definitions. we still need the error handling when applying ClickHouseInstallation CR though.

you can use a more concise style for those:

if _, err := data.PodWaitFor(defaultTimeout, podName, kubeNamespace, func(p *corev1.Pod) (bool, error) {
    ...
}); err != nil {
    return nil, err
}

introduced a helper function to wait for ready status as @dreamtalen 's comment below.

I just realized we are using the Pod IP to configure the Flow Aggregator. Do you know why we don't use the default instead (the Service DNS)?

previously the dns name may have issue when tested together with other Antrea kind e2e test modes. reverted to the default svc dns name as the case is isolated now.

should we get the logs from the CH operator Pod as well, just in case?

added

I think the Flow Visibility NS creation is part of flowVisibilityYML, did you delete that intentionally in the e2e mode of generating manifest script?

The ns is defined under grafana yml which is not included as base here. I think maybe we can keep it as-is and revist this after theia migration.

I don't really like assuming that curl is available on the Nodes, but the only other solution is to start a Pod just to curl...

from kind node image dockerfile https://github.com/kubernetes-sigs/kind/blob/main/images/base/Dockerfile
we can see curl has been there for more than 3 years and actively being used as part of image build process. given its small size and common use case, i think the possibility of them suddenly remove it from the node image is quiet low. it's also used the same way in part of current antctl e2e test. given that fa tests are isolated now I think maybe we can always come back and and switch to the pod approach later.

do we use log.Info in other places?

done. it's used in several other places under the same file. for example:
func killProcessAndCollectCovFiles: log.Infof("Sending SIGINT to '%s'", processName)
func DeleteNamespace: log.Infof("Deleting Namespace %s took %v", namespace, time.Since(startTime))

FlowExporter?

done